Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support regional and custom cosmetic filters #4700

Merged
merged 3 commits into from
Feb 27, 2020
Merged

Support regional and custom cosmetic filters #4700

merged 3 commits into from
Feb 27, 2020

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Feb 20, 2020 •
edited
Loading

Resolves brave/brave-browser#4348

Submitter Checklist:

Test Plan:

I have added some new unit tests to verify that the logic for merging cosmetic filter information from the different sources works as expected.

Manual verification of custom filter application:

  1. Open this sample page
  2. Ensure there is no blank space under the word "Advertisement" on the right column (default adblock lists)
  3. Open brave://adblock in a new tab
  4. At the bottom, add the following custom rule to the "Custom Filters" input: myspace.com###articleLeft
  5. Return to the sample page and refresh
  6. Ensure that there is still no blank space under the "Advertisement" label in the right column
  7. Ensure that the text of the article is also hidden

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@antonok-edm antonok-edm self-assigned this Feb 20, 2020
@antonok-edm antonok-edm requested review from bbondy, pes10k, emerick and petemill and removed request for pes10k February 21, 2020 04:12
emerick
emerick reviewed Feb 21, 2020
View reviewed changes
components/brave_shields/browser/ad_block_base_service.cc Outdated Show resolved Hide resolved
components/brave_shields/browser/ad_block_base_service.cc Outdated Show resolved Hide resolved
components/brave_shields/browser/ad_block_base_service.cc Outdated Show resolved Hide resolved
components/brave_shields/browser/ad_block_regional_service_manager.cc Outdated Show resolved Hide resolved
components/brave_shields/browser/ad_block_regional_service_manager.cc Show resolved Hide resolved
components/brave_shields/browser/ad_block_regional_service_manager.cc Outdated Show resolved Hide resolved
components/brave_shields/browser/ad_block_regional_service_manager.cc Show resolved Hide resolved
components/brave_shields/browser/cosmetic_merge_unittest.cc Outdated Show resolved Hide resolved
components/brave_shields/browser/cosmetic_merge_unittest.cc Outdated Show resolved Hide resolved
@antonok-edm
Copy link
Collaborator Author

@emerick everything's updated according to your feedback

emerick
emerick approved these changes Feb 21, 2020
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bsclifton bsclifton added this to the 1.7.x - Nightly milestone Feb 23, 2020
@bsclifton
Copy link
Member

@antonok-edm besides automated tests, are there some manual test steps you can provide for QA to try this out?

@pes10k
Copy link
Contributor

pes10k commented Feb 23, 2020

@antonok-edm is there an example from the crawl you did over the summer / autumn you could pull out and just copy-paste the URL here? I think that would suffice.

@antonok-edm
Copy link
Collaborator Author

@bsclifton @pes10k I've updated the Test Plan to include the example I used to make sure this works for custom filters. Unfortunately I haven't been able to find any good examples of up-to-date cosmetic filters in regional lists, but it uses the same machinery.

@pes10k
Copy link
Contributor

pes10k commented Feb 23, 2020

Terrific! @bsclifton for my two cents, I think @antonok-edm's test plan is sufficient to make sure the relevant functionality is tickled

@bsclifton
Copy link
Member

Updated test plan looks perfect! 😄

@srirambv srirambv mentioned this pull request Feb 23, 2020
Closed
@antonok-edm antonok-edm merged commit dce99ff into master Feb 27, 2020
@antonok-edm antonok-edm deleted the cosmetic-regional-and-custom branch February 27, 2020 04:36
brave-builds pushed a commit that referenced this pull request Feb 27, 2020
@brave-browser-releases
Uplift of #4700 (squashed) to dev
1dbae02
@brave-builds brave-builds mentioned this pull request Feb 27, 2020
Merged
4 tasks
@kjozwiak
Copy link
Member

kjozwiak commented Mar 3, 2020

Verification PASSED on macOS 10.15.3 x64 using the following build:

Brave 1.7.35 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
Revision cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS macOS Version 10.15.3 (Build 19D76)

Without the custom filter

Screen Shot 2020-03-03 at 11 37 59 AM

Using the custom filter

Screen Shot 2020-03-03 at 11 38 30 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emerick emerick emerick approved these changes

@bbondy bbondy Awaiting requested review from bbondy

@petemill petemill Awaiting requested review from petemill

@pes10k pes10k Awaiting requested review from pes10k

Assignees

@antonok-edm antonok-edm

Labels
None yet
Projects
None yet
Milestone
1.7.x - Release
Development

Successfully merging this pull request may close these issues.

Custom adblock rules should allow cosmetic filters
5 participants
@antonok-edm @bsclifton @pes10k @kjozwiak @emerick